Skip to content

Conversation

nlebovits
Copy link
Contributor

@nlebovits nlebovits commented Sep 28, 2025

Related Issue(s):

Description:

  • Create dedicated tests/test_warnings.py file for warning context manager tests
  • Add comprehensive test coverage for ignore() and strict() context managers
  • Fix warnings.py docstring example to use imperfect-api.test for clarity
  • Fix collection_search.py to pass warning objects directly to warnings.warn() instead of string concatenation
  • Add pytest warning filters to prevent CI failures from pre-existing socket warnings
  • Achieve 100% test coverage for warnings.py module

PR Checklist:

  • Code is formatted
  • Tests pass
  • Changes are added to CHANGELOG.md

nlebovits and others added 2 commits September 28, 2025 16:57
- Add ignore() tests to all test classes that previously only tested strict()
- Use get_collections() for ignore() tests since it emits warnings (unlike search() which raises exceptions directly)
- Achieve 100% coverage for warnings.py module
Copy link
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on increasing the test coverage! But I don't think these make sense as additions to preexisting search API tests. Maybe they deserve their own class for get_collections. Also you might need to add an ignore to cover the ResourceWarning that is showing up in CI

- Add ignore() tests to all test classes that previously only tested strict()
- Use get_collections() for ignore() tests since it emits warnings (unlike search() which raises exceptions directly)
- Achieve 100% coverage for warnings.py module
Remove all ignore() test blocks that were mixed with API tests.
These tests now belong in the dedicated test_warnings.py file.
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.06%. Comparing base (21435b0) to head (bd83e0d).
⚠️ Report is 152 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #832      +/-   ##
==========================================
+ Coverage   93.43%   95.06%   +1.63%     
==========================================
  Files          13       15       +2     
  Lines         990     1217     +227     
==========================================
+ Hits          925     1157     +232     
+ Misses         65       60       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Ignore ResourceWarning and PytestUnraisableExceptionWarning to prevent
CI failures from pre-existing unclosed socket warnings that are not
related to our warning context manager tests.
@nlebovits nlebovits force-pushed the lebovits/590-increase-test-coverage branch from 26bf119 to 5221373 Compare October 4, 2025 16:47
@nlebovits
Copy link
Contributor Author

nlebovits commented Oct 4, 2025

@jsignell thanks for the review! I think I have my head wrapped around it now. I rolled back my previous changes and added a separate test file for the warnings context managers based on input from @gadomski. Tests and CI are now passing.

nlebovits and others added 2 commits October 4, 2025 14:00
Add entry documenting the comprehensive test coverage added for
ignore() and strict() context managers in PR stac-utils#832.
Copy link
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Just one little comment, but looks good!

@jsignell jsignell enabled auto-merge (squash) October 9, 2025 16:25
@jsignell jsignell merged commit 0a68712 into stac-utils:main Oct 9, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants